Skip to content

Conversation

sunyuhan1998
Copy link
Contributor

As mentioned in the issue, TemplateRenderer currently only supports rendering templates. The ability to also parse and extract the required variables from a given template would be highly beneficial, especially for proactively validating template correctness. This PR implements that capability.

Specifically, it:

  1. Adds a getRequiredVariables method to TemplateRenderer to extract the variable names required by the template;

  2. Includes corresponding unit tests to ensure correctness and stability.

Fixes #4251

…TemplateRenderer` to parse the required variables from the template.

Signed-off-by: Sun Yuhan <sunyuhan1998@users.noreply.github.com>
# Conflicts:
#	spring-ai-model/src/test/java/org/springframework/ai/chat/prompt/SystemPromptTemplateTests.java
Signed-off-by: Sun Yuhan <sunyuhan1998@users.noreply.github.com>
Signed-off-by: Sun Yuhan <sunyuhan1998@users.noreply.github.com>
@ericbottard
Copy link
Member

Hi @sunyuhan1998,
thanks for your contribution!

Could you please rebase against main and also add the following logic: not all implementations may support retrieving variable names. As a consequence, could you make the new method throw NotSupportedException and make it default

# Conflicts:
#	spring-ai-template-st/src/test/java/org/springframework/ai/template/st/StTemplateRendererTests.java
…to handle cases where subclasses do not support this method.

Signed-off-by: Sun Yuhan <sunyuhan1998@users.noreply.github.com>
@sunyuhan1998
Copy link
Contributor Author

Could you please rebase against main and also add the following logic: not all implementations may support retrieving variable names. As a consequence, could you make the new method throw NotSupportedException and make it default

Thank you very much for your review. I agree with your comments. I have merged the code from the main branch and adjusted the getRequiredVariables method as you suggested. However, I couldn't seem to find the NotSupportedException class in the project, so I used OperationNotSupportedException instead. Do you think this is acceptable? Could you please review it once again? Thank you so much!

@ericbottard
Copy link
Member

Could you please rebase against main and also add the following logic: not all implementations may support retrieving variable names. As a consequence, could you make the new method throw NotSupportedException and make it default

Thank you very much for your review. I agree with your comments. I have merged the code from the main branch and adjusted the getRequiredVariables method as you suggested. However, I couldn't seem to find the NotSupportedException class in the project, so I used OperationNotSupportedException instead. Do you think this is acceptable? Could you please review it once again? Thank you so much!

That's the one I meant, sorry.

However, the more I look at this, the more I think this may be a rabbit hole, especially for the case you want to support: what about nested properties? what about method invocations on objects? what about x... ?
Each templating library (granted we only have one at the moment, but you opened a PR to submit one with jinja and I saw one using SpEL) may support different and complex semantics of things they can do with the model. The caller of TemplateRenderer is not capable of deciding whether the implementation will fail or not, in the general case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request templating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TemplateRenderer should support retrieving required variables from the template.
3 participants